-
-
Notifications
You must be signed in to change notification settings - Fork 13.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
otree: init at 0.2.0 #316252
base: master
Are you sure you want to change the base?
otree: init at 0.2.0 #316252
Conversation
bff636b
to
ad6dd98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, @0x61nas! 😃 👍
Please find below a few comments and suggestions, mainly to add support for building on darwin.
Is there any way we could remove the dependency on git for building? Upstream seems to offer BUILD_OTREE_WITH_GIT_INFO
and CARGO_PKG_VERSION
could those be helpful here?
pkgs/by-name/ot/otree/package.nix
Outdated
repo = "otree"; | ||
rev = "v${version}"; | ||
hash = "sha256-ukKlypGY0+xsci/loAnf0Sz5K+82gVzkb+KnZ139cIA="; | ||
leaveDotGit = true; # needed by build.rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ I was able to successfully build on aarch64-darwin
without this.
in v0.1.0 i dont think so
oh, that's would help ofc, i think with this we can remove |
Maybe a TODO comment in the package regarding removal of the |
good idea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment below…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this so quickly, @0x61nas, much appreciated!
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4071 |
Co-authored-by: Alexis Hildebrandt <afh@surryhill.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How fortunate, that version 0.2.0 was released while this PR is still under review and leaveDotGit
and git
in nativeBuildInputs
are no longer necessary 🥳
If you wanted to go above and beyond with this PR, please consider formatting it using:
nix run github:NixOS/nixfmt -- pkgs/by-name/ot/otree/package.nix
@0x61nas any idea what could cause the ofborg-eval check failure? |
at the first, i thought that it was because of our (hacky) featching, but now i am not sure why |
Format using nixfmt-rfc-style |
Description of changes
addition otree package, which is a command line tool to view objects (json/yaml/toml) in TUI tree widget
try the package
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.